Implement GMP operator type specifying extension#5223
Implement GMP operator type specifying extension#5223Firehed wants to merge 8 commits intophpstan:2.1.xfrom
Conversation
|
You've opened the pull request against the latest branch 2.2.x. PHPStan 2.2 is not going to be released for months. If your code is relevant on 2.1.x and you want it to be released sooner, please rebase your pull request and change its target to 2.1.x. |
Firehed
force-pushed
the
gmp-operator-type-extension
branch
from
a7b2630 to
c75bac6
Compare
| $specifiedTypes = $this->operatorTypeSpecifyingExtensionRegistryProvider->getRegistry() | ||
| ->callOperatorTypeSpecifyingExtensions($expr, $leftType, $rightType); | ||
| if ($specifiedTypes !== null) { | ||
| return $specifiedTypes; | ||
| } |
There was a problem hiding this comment.
This become a duplicate with the check later
$specifiedTypes = $this->operatorTypeSpecifyingExtensionRegistryProvider->getRegistry()
->callOperatorTypeSpecifyingExtensions($expr, $leftType, $rightType);
if ($specifiedTypes !== null) {
return $specifiedTypes;
}
I would either:
- remove this and rely on the later call
- or just move the existing call first with condition on object.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| $leftType = $getTypeCallback($left); | ||
| $rightType = $getTypeCallback($right); | ||
|
|
||
| if ($leftType->isObject()->yes() || $rightType->isObject()->yes()) { |
There was a problem hiding this comment.
This shouldn't check for Object and be done unconditionally.
| // GMP supports unary minus and returns GMP | ||
| if ($type->isObject()->yes() && (new ObjectType('GMP'))->isSuperTypeOf($type)->yes()) { | ||
| return new ObjectType('GMP'); | ||
| } |
There was a problem hiding this comment.
Rather than an hardcoded check for this, I feel like it we should introduce an UnaryOperatorTypeSpecifyingExtension as explained here: #4980 (comment)
This is maybe better in a dedicated PR ?
| $exprType = $getTypeCallback($expr); | ||
|
|
||
| // GMP supports bitwise not and returns GMP | ||
| if ($exprType->isObject()->yes() && (new ObjectType('GMP'))->isSuperTypeOf($exprType)->yes()) { |
There was a problem hiding this comment.
Same for UnaryOperatorTypeSpecifyingExtension
|
Updated to target 2.1 instead of 2.2 per the bot's suggestion. The two failing-as-of-writing tests appear to be failing on other PRs so I don't think they're specific to this change. I'll check out the other feedback shortly. Thank you for the very quick review! |
|
Please introduce the changes to InitializerExprTypeResolver in a separate PR - this will force you to create synthetic extensions just for test purposes, and review these changes separately, which I want you to do. Without this, if we once removed the GMP extensions then this feature would become untested. After that is merged, then you can take advantage of it with new GMP extensions in a new PR. |
|
Can do. Thanks for the feedback! |
|
Split per feedback: #5226 contains the |
Firehed
force-pushed
the
gmp-operator-type-extension
branch
from
ade4b49 to
4442956
Compare
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Nice.
Now please again, split this PR in two:
- One with the GmpOperatorTypeSpecifyingExtension
- One with the unary minus, unary plus and bitwise not operator
Because for the second one, I think it will be a better API to introduce an UnaryOperatorTypeSpecifyingExtension
|
Gotcha, will do. Missed your message before continuing. |
|
@VincentLanglet #5284 sets up the UnaryOperatorTypeSpecifyingExtension; once that's looking good I'll update the GMP code to build on it instead. |
It's merged, you can now rebase/update this PR :) |
This adds comprehensive type inference tests for GMP operations: - Arithmetic operators (+, -, *, /, %, **) with GMP on left and right - Bitwise operators (&, |, ^, ~, <<, >>) with GMP on left and right - Comparison operators (<, <=, >, >=, ==, !=, <=>) with GMP on left and right - Assignment operators (+=, -=, *=) - Corresponding gmp_* functions (gmp_add, gmp_sub, gmp_mul, etc.) These tests currently fail because PHPStan lacks a GmpOperatorTypeSpecifyingExtension to specify that GMP operations return GMP rather than int|float. Related: phpstan/phpstan#12123 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add GmpOperatorTypeSpecifyingExtension to properly infer return types for GMP operator overloads. GMP supports arithmetic (+, -, *, /, %, **), bitwise (&, |, ^, ~, <<, >>), and comparison (<, <=, >, >=, ==, !=, <=>) operators. The extension only claims support when both operands are GMP-compatible (GMP, int, or numeric-string). Operations with incompatible types like stdClass are left to the default type inference. Also update InitializerExprTypeResolver to call operator extensions early for object types in resolveCommonMath and bitwise methods, and add explicit GMP handling for unary operators (-$a, ~$a). Fixes phpstan/phpstan#14288 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements UnaryOperatorTypeSpecifyingExtension for GMP to handle unary minus (-), unary plus (+), and bitwise NOT (~) operators. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Firehed
force-pushed
the
gmp-operator-type-extension
branch
from
aafd495 to
eaebe39
Compare
|
Alright @VincentLanglet I've done the rebase again, and now this is purely the GMP stuff leveraging the type overloading stuff from the previous two split-off PRs. It's still pretty much entirely Claude going at it, but it's now feeling pretty tightly-scoped from my perspective. LMK if there's anything else here :) |
- Simplify isOperatorSupported to only check if one side is GMP - Move compatibility checking to specifyType, returning ErrorType for incompatible operands (like stdClass) - Add unit tests for the extension - Update pow.php test to expect ErrorType for stdClass ** GMP Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You just need to fix the build |
- Add #[Override] attribute to setUp() method - Replace match expression with switch for PHP 7.4 compatibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use ObjectWithoutClassType for generic object type (not ObjectType('object'))
- Add test cases for object+GMP to catch IsSuperTypeOfCalleeAndArgumentMutator on line 37
- Add test case for GMP|int+int to catch TrinaryLogicMutator on line 37
- Add test cases for GMP+int|stdClass to catch TrinaryLogicMutator on line 52
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add GMP + string test to catch TrinaryLogicMutator on isNumericString - Add unary operator tests on object type to catch mutations in GmpUnaryOperatorTypeSpecifyingExtension Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
General string has isNumericString()=Maybe, catching the TrinaryLogicMutator that changes .yes() to !.no() on line 53. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Ok, I can't for the life of me figure out the mutation testing issues, even after going through setting it up locally and ensuring it passed there. Perhaps it's somehow specific to 8.3/8.4 vs my 8.5? I will probably need an assist here to get it over the line. I don't think the Larastan issues are related to this PR based on the errors, though I also don't see it failing on the upstream branch. It's a bit tricky to confirm since CI generates an enormous number of jobs (how do you all get such good parallelism btw? I see my own job matrixes cap out way lower, including for OSS projects) |
|
@Firehed PHPStan org pays for the enterprise plan 😊 |
|
There is sometimes false positives about mutation testing and larastan is unrelated indeed so I would say it's ready @Firehed |
|
This pull request has been marked as ready for review. |
|
Alright @VincentLanglet - moved out of draft state. LMK if you want any other edits. Thanks for the info on CI @ondrejmirtes. And thanks to both of you for the support and feedback getting this wrapped up! |
| } | ||
|
|
||
| $gmpType = new ObjectType('GMP'); | ||
| return $gmpType->isSuperTypeOf($operand)->yes(); |
There was a problem hiding this comment.
the
- return $gmpType->isSuperTypeOf($operand)->yes();
+ return !$gmpType->isSuperTypeOf($operand)->no();
mutation can be killed by testing with a "maybe GMP operand", like
function maybeGmp(GMP $a, GMP $b) {
if (rand(0,1)) {
$a = 5;
}
assertType(..., $a + $b));
}same for the other extension
|
running PHPStan on gmp code with this PR leads errors like repro |
The error are coming from the strict rules no ? I would say that the strict rules will need to be updated once the operator type specifying extension is merged |
I think we should already prepare the strict-rules PR - before merging here. |
I don't think it's a release blocker considering this: The This PR is fixing the second point without touching the first one. |
5 participants
Summary
GmpOperatorTypeSpecifyingExtensionto properly infer return types for GMP operator overloadsInitializerExprTypeResolverto call operator extensions early for object typesFixes phpstan/phpstan#14288
Test plan
tests/PHPStan/Analyser/nsrt/gmp-operators.phpgmp_*functions🤖 Generated with Claude Code